Add additional support for parachute #2063
Conversation
There was a problem hiding this comment.
Thanks for the pull request!
Now, we're trying to avoid having specific messages for every component type and try to cover it with more generic component messages. For specific configuration, we can then use parameters. I have made a few inline comments around that.
The risk with specific configuration flags is that every new device/implementation of a parachute system might have slightly different settings and require more flags and status fields.
|
@julianoes I understand @amy-chen-skydio has modified/responded to your comments. Can you please recheck? |
message_definitions/v1.0/common.xml
Outdated
| @@ -1764,6 +1764,15 @@ | |||
| <param index="6">Empty</param> | |||
| <param index="7">Empty</param> | |||
| </entry> | |||
| <entry value="215" name="MAV_CMD_SET_PARACHUTE_ARM" hasLocation="false" isDestination="false"> | |||
There was a problem hiding this comment.
Shouldn't the arming / disarming be done through the message
https://mavlink.io/en/messages/common.html#MAV_CMD_COMPONENT_ARM_DISARM
and the flags turned into parameters, as @julianoes suggested?
There was a problem hiding this comment.
This is a bit different, parachute arming is a bit more modal than the ARM_DISARM command and can change flight to flight as well as within a flight. I would advocate to keep this as is.
Example: A parachute can be armed in auto-tigger mode where it detects a fault and triggers or armed in a mode where only a manual trigger / button press would deploy the parachute. This could change several times throughout a flight depending on the user application
I think this could also be solved by adding a parameter to ARM_DISARM but guessing that we don't want to change that command since its used for autopilots which do not seem to need a parameter
message_definitions/v1.0/common.xml
Outdated
| <field type="uint8_t" name="arm_status" enum="PARACHUTE_ARM_FLAGS" display="bitmask">Parachute arming status</field> | ||
| <field type="uint8_t" name="charge_status">Boolean indicating charging (1) or not (0)</field> | ||
| <field type="uint8_t" name="deployment_status" enum="PARACHUTE_DEPLOYMENT_FLAGS" display="bitmask">Parachute deployment_status</field> | ||
| <field type="uint8_t" name="ats_arm_altitude" units="m">Parachute ATS auto-arming/disarming altitude in meters</field> |
There was a problem hiding this comment.
I don't think Automatic Trigger System (ATS) is a common abbreviation.
| <field type="uint8_t" name="ats_arm_altitude" units="m">Parachute ATS auto-arming/disarming altitude in meters</field> | |
| <field type="uint8_t" name="ats_arm_altitude" units="m">Parachute Automatic Trigger System (ATS) auto-arming/disarming altitude in meters</field> |
There was a problem hiding this comment.
Made the change in the latest commit!
message_definitions/v1.0/common.xml
Outdated
| <description>There is an error with the parachute external power source</description> | ||
| </entry> | ||
| <entry value="2048" name="PARACHUTE_ERROR_FLAGS_OBC_CONNECTION_ERROR"> | ||
| <description>There is an error with the parachute and OBC (onboard computer) connection</description> |
There was a problem hiding this comment.
- "onboard" is typically used for computers inside of the component. For instance a camera that has its own compute unit that can do more than just store a photo on an SD card.
- "offboard" was historically used for a standalone computer that is not the autopilot.
That's why I'm not sure what the intended meaning in this PR is.
There was a problem hiding this comment.
understood. This should be offboard throughout
There was a problem hiding this comment.
@jenke-skydio how is the connection to the offboard computer defined? How should a parachute monitor its state? Is the parachute expected to look for heartbeat messages that have the MAV_TYPE_ONBOARD_CONTROLLER type?
There was a problem hiding this comment.
connection to offboard computer is optional, when its there it must send HEARTBEAT and optionally SYSTEM_TIME
There was a problem hiding this comment.
There isn't really a place in in xml file for this kind of information. I expect you'll need to create a parachute service doc that explains the models and use cases that are addressed.
There was a problem hiding this comment.
yes 100%. We plan to write a up parachute microservice documentation in the MAVLink devguide
amy-chen-skydio
force-pushed
the
skydio-add-parachute-support
branch
from
2bf5a45
to
cb77a8b
Compare
|
@potaito @julianoes There has been a round of changes to this. Can you please re-review and confirm that your concerns have been addressed, and highlight any that have not. @amy-chen-skydio Thanks for your patience. When this converges, I have the same comments to this as in this comment for the illuminators PR (how to validate/documentation required): #2047 (comment) |
message_definitions/v1.0/common.xml
Outdated
| <description>There is an error with the parachute external power source</description> | ||
| </entry> | ||
| <entry value="2048" name="PARACHUTE_ERROR_FLAGS_OBC_CONNECTION_ERROR"> | ||
| <description>There is an error with the parachute and OBC (onboard computer) connection</description> |
There was a problem hiding this comment.
understood. This should be offboard throughout
message_definitions/v1.0/common.xml
Outdated
| @@ -1764,6 +1764,15 @@ | |||
| <param index="6">Empty</param> | |||
| <param index="7">Empty</param> | |||
| </entry> | |||
| <entry value="215" name="MAV_CMD_SET_PARACHUTE_ARM" hasLocation="false" isDestination="false"> | |||
There was a problem hiding this comment.
This is a bit different, parachute arming is a bit more modal than the ARM_DISARM command and can change flight to flight as well as within a flight. I would advocate to keep this as is.
Example: A parachute can be armed in auto-tigger mode where it detects a fault and triggers or armed in a mode where only a manual trigger / button press would deploy the parachute. This could change several times throughout a flight depending on the user application
I think this could also be solved by adding a parameter to ARM_DISARM but guessing that we don't want to change that command since its used for autopilots which do not seem to need a parameter
|
@julianoes @potaito Can you please respond to @jenke-skydio 's comments? |
@potaito Wanted to follow up to see if you've had a chance to look over @jenke-skydio's comments. Thank you! |
- Add messages to report parachute status and information - Add commands to arm with additional trigger methods and to shutdown the module
- Add `MAV_CMD_SET_PARACHUTE_ARM_ALTI` for setting the ATS arm altitude - Add `PARACHUTE_ERROR_FLAGS_GS_ERROR` and `PARACHUTE_ERROR_FLAGS_SUBSYSTEM_ERROR` in `PARACHUTE_ERROR_FLAGS`
- Remove `MAV_CMD_DO_PARACHUTE_SHUTDOWN` and use `MAV_CMD_PREFLIGHT_REBOOT_SHUTDOWN` instead
- Remove `battery_status` from `PARACHUTE_STATUS` and use `BATTERY_STATUS` instead
- Remove `PARACHUTE_INFORMATION` and use `COMPONENT_INFORMATION_BASIC` instead
- Add `manufacture_date` to `COMPONENT_INFORMATION_BASIC`
- Move `parachute_packed_date` to `PARACHUTE_STATUS`
Address PR feedback and test failures - Move parachute `MAV_CMD` enums since already in-use by ardupilotmega - Clean up spurious whitespace - ATS (automatic trigger system) isn't a common abbreviation - Removed manufacture date addition from this PR
Address PR feedback: - Remove `MAV_CMD_SET_PARACHUTE_ARM_ALTI`, using params instead - Rename `PARACHUTE_ARM_FLAGS` to `PARACHUTE_TRIGGER_FLAGS` and update descriptions - Rename `PARACHUTE_DEPLOYMENT_FLAGS` to `PARACHUTE_DEPLOYMENT_TRIGGER`, update descriptions, and use it as enum instead of bit flags - Update descriptions in `PARACHUTE_ERROR_FLAGS` - Remove `charge_status` from `PARACHUTE_STATUS`, using `BATTERY_STATUS` instead New items added for safety and regulatory purposes: - `PARACHUTE_SAFETY_FLAGS` and FTS pre-checking
amy-chen-skydio
force-pushed
the
skydio-add-parachute-support
branch
from
cb77a8b
to
f59f2b5
Compare
|
@julianoes @potaito I believe that @amy-chen-skydio has addressed all your concerns. To my mind the descriptions for each trigger source are now clear, and this makes sense. Are you OK with it? |
|
@amy-chen-skydio Assuming that everyone is happy with this, we'll need similar tests/validations as for the illuminators to get this in. |
Update `MAV_CMD_SET_PARACHUTE_ARM` description Co-authored-by: Hamish Willee <hamishwillee@gmail.com>
message_definitions/v1.0/common.xml
Outdated
| <description>There is an error with the parachute IMU</description> | ||
| </entry> | ||
| <entry value="4" name="PARACHUTE_ERROR_FLAGS_RF_CONNECTION_ERROR"> | ||
| <description>There is an error with the parachute RF (used for manual control)</description> |
There was a problem hiding this comment.
| <description>There is an error with the parachute RF (used for manual control)</description> | |
| <description>There is an error with the parachute's RF device used for manual control</description> |
There was a problem hiding this comment.
Updated the wording to There is an error with the parachute's RF that is used for manual control
message_definitions/v1.0/common.xml
Outdated
| <description>Geofence</description> | ||
| </entry> | ||
| <entry value="6" name="PARACHUTE_DEPLOYMENT_TRIGGER_FTS_PRECHECKING"> | ||
| <description>FTS (flight termination system) pre-checking protocol</description> |
There was a problem hiding this comment.
| <description>FTS (flight termination system) pre-checking protocol</description> | |
| <description>Flight Termination System (FTS) pre-checking protocol</description> |
Address PR feedback: - Updated `parachute_packed_date` date string to ISO 8601 format - Updated descriptions for cleaner phrasing
|
@potaito All your comments addressed? |
|
@hamishwillee But it's not a deal breaker, so LGTM. Thanks for the patience. |
@hamishwillee should I upload the tests and microservice here or should we create a PR for mavlink-devguide directly? |
|
Update `PARACHUTE_DEPLOYMENT_TRIGGER_NONE` description Co-authored-by: Hamish Willee <hamishwillee@gmail.com>
|
Excellent. So from my perspective this is just waiting on docs - for the tests. |
MAV_CMD_SET_PARACHUTE_ARMto arm parachute with additional triggering methods